Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

Reduce height of Brand and Script Link components #2569

Merged
merged 25 commits into from
Nov 12, 2019
Merged

Reduce height of Brand and Script Link components #2569

merged 25 commits into from
Nov 12, 2019

Conversation

OlgaLyubin
Copy link
Contributor

@OlgaLyubin OlgaLyubin commented Nov 5, 2019 •
edited
Loading

Resolves #2555

Overall change:
Changed the height of Script Link component for breakpoints above 600px and the height of Brand Component for all breakpoints.

Code changes:

  • Changed styling for Script Link and Brand Component
  • Updated snapshots
  • I have assigned myself to this PR and the corresponding issues
  • Automated jest tests added (for new features) or updated (for existing features)
  • This PR requires manual testing

@OlgaLyubin OlgaLyubin added ws-home Tasks for the WS Home Team ws-fp-phase3 labels Nov 5, 2019
@OlgaLyubin OlgaLyubin added this to the Navigation (WS FP) milestone Nov 5, 2019
@OlgaLyubin OlgaLyubin self-assigned this Nov 5, 2019
@OlgaLyubin OlgaLyubin added the ux To be reviewed by UX before merging label Nov 5, 2019
tochwill
tochwill previously approved these changes Nov 7, 2019
View reviewed changes
@DenisHdz
Copy link
Contributor

DenisHdz commented Nov 7, 2019

The Brand doesn't look to be vertically centered inside the Banner. Is that expected?

brand

brand2

@OlgaLyubin
Copy link
Contributor Author

@DenisHdz
Originally the Brand was implemented in such a way that it is centred, but top and bottom paddings work in such a way that bottom padding is 4px smaller than the top padding, however it's compensated with the height of the piece of red banner underneath. I guess this is because if you hover over the Brand, that little piece of red banner becomes white to signify a link. So in terms of pixels, the Brand is centred within the Banner. Here is a screenshot of Storybook from latest :

Screenshot 2019-11-07 at 12 19 16

I just followed the same logic in my PR.

@OlgaLyubin @DenisHdz
Refactoring
25d4ea5 
Co-Authored-By: Denis Hernandez <46446236+DenisHdz@users.noreply.github.com>
DenisHdz
DenisHdz approved these changes Nov 8, 2019
View reviewed changes
Copy link
Contributor

@DenisHdz DenisHdz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

sareh
sareh approved these changes Nov 11, 2019
View reviewed changes
Copy link
Contributor

@sareh sareh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good. We should also get this UX reviewed prior to merging.

@OlgaLyubin
Copy link
Contributor Author

This PR has already been reviewed and approved by UX

@PriyaKR PriyaKR self-assigned this Nov 11, 2019
@PriyaKR
Copy link
Contributor

PriyaKR commented Nov 12, 2019

Looks good to me.

@PriyaKR PriyaKR merged commit e1ca919 into latest Nov 12, 2019
@PriyaKR PriyaKR deleted the change-brand-height branch November 12, 2019 11:07
@OlgaLyubin OlgaLyubin removed this from the Navigation (WS FP) milestone Nov 15, 2019
@bbc-news-frameworks bbc-news-frameworks mentioned this pull request Jan 13, 2020
Merged
@PriyaKR PriyaKR mentioned this pull request Jan 24, 2020
Merged
5 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sareh sareh sareh approved these changes

@tochwill tochwill tochwill approved these changes

@ryanmccombe ryanmccombe ryanmccombe approved these changes

@Bopchy Bopchy Bopchy approved these changes

@DenisHdz DenisHdz DenisHdz approved these changes

@jamesbhobbs jamesbhobbs Awaiting requested review from jamesbhobbs

@dr3 dr3 Awaiting requested review from dr3

@j-pendlebury j-pendlebury Awaiting requested review from j-pendlebury

@jroebu14 jroebu14 Awaiting requested review from jroebu14

Assignees

@PriyaKR PriyaKR

@OlgaLyubin OlgaLyubin

Labels
ux To be reviewed by UX before merging ws-home Tasks for the WS Home Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reduce height of Brand and Script Link components
7 participants
@OlgaLyubin @DenisHdz @PriyaKR @sareh @tochwill @ryanmccombe @Bopchy